scripts(configure_dev): conditional tap homebrew#5573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5573 +/- ##
=======================================
Coverage 55.79% 55.79%
=======================================
Files 446 446
Lines 63418 63418
=======================================
+ Hits 35381 35386 +5
+ Misses 25668 25667 -1
+ Partials 2369 2365 -4 see 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
LGTM.
Homebrew 2.4.0 is over 3 years old, I'd be tempted to remove this line entirely.
There was a problem hiding this comment.
I'm less comfortable with this change than the original for the following reasons:
- Much more code. AFAIK the current code isn't specifically broken, so I'm generally a bit hesitant about seeing this code change.
- It doesn't work for the following inputs:
compare_semantic_version "2.5.0" "",compare_semantic_version "2.5.0" "2". - There are too many print statements.
Instead of a semantic version check, perhaps testing the exit code of brew casks can be used? I'm not a brew user, so I found that command in the docs.
|
In general, you are right @winder! Since inputs are expected to be semantic versions and "" and "2" are not semantic versions they produce wrong responses! I will tweak the code and address all your mentioned points! Thank you! |
|
And current code breaks on new brew versions (in some regions because it was ok over VPN to the USA) and that's why I made this change! I was running from source code on Ubuntu so far and this was the first time I tried on MacOS and configure_dev.sh script failed! I thought it's worth mentioning! |
|
Hey @winder : now it is 0 message and just silently does the job and no additional code file and least code added (4 lines only) |
|
@winder my man, any chance you can take a look at this very humble PR status? Million thanks in advance my friend! |
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
Thanks for bearing with us. The improvements look good.
|
@bbroder-algo , @winder , Thank you both for your time! The PR is not merged yet! |
4 participants
Summary
In OSX, homebrew does not need to tap in versions above "2.5.0", so this PR adds a condition to check that before running
brew tap homebrew/caskand does not proceed with tap for homebrew versions above "2.5.0"Test Plan
Tested on an old and new MacOS with homebrew versions above and below 2.5.0 and it does detect and work fine!
Dear reviewers, please inform me if there is a need to make a unit test using frameworks like bats or shunit2! But since it was just a small modification I humbly considered It would not be necessary!